Skip to content

refactor(proto)!: use well-known time types - #3113

Open
derekwaynecarr wants to merge 1 commit into
NVIDIA:mainfrom
derekwaynecarr:refactor/3052-use-well-known-time-types/derekwaynecarr
Open

refactor(proto)!: use well-known time types#3113
derekwaynecarr wants to merge 1 commit into
NVIDIA:mainfrom
derekwaynecarr:refactor/3052-use-well-known-time-types/derekwaynecarr

Conversation

@derekwaynecarr

@derekwaynecarr derekwaynecarr commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Replace public protobuf timestamp and duration scalars with the standard google.protobuf.Timestamp and google.protobuf.Duration types. This gives API consumers consistent time semantics, preserves sub-second precision, and distinguishes absent optional values from zero-valued times and durations.

Related Issue

Closes #3052

Changes

  • Replaced epoch-millisecond, epoch-string, duration-string, and duration-second protobuf fields with well-known time types while reserving the retired field names and numbers.
  • Added shared Rust helpers for validating and converting protobuf timestamps and durations without losing nanosecond precision.
  • Added transactional startup migration for legacy protobuf payloads stored in SQLite and PostgreSQL, including nested messages and timestamp-valued maps.
  • Updated gateway, CLI, sandbox, compute drivers, middleware, TUI, and provider flows to consume and produce the new fields.
  • Updated the Go, TypeScript, and Python SDK implementations and tests for the new wire representation while preserving their curated public APIs where applicable.
  • Documented the protobuf time conventions, compatibility behavior, and persistence boundary.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture and user-facing documentation are updated

Signed-off-by: Derek Carr <decarr@redhat.com>
@derekwaynecarr
derekwaynecarr requested review from a team, mrunalp and sjenning as code owners September 1, 2026 22:12
@copy-pr-bot

copy-pr-bot Bot commented Sep 1, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@derekwaynecarr

Copy link
Copy Markdown
Collaborator Author

/ok to test 62e0845

@drew drew left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall design is sound: replacing scalar time fields with protobuf well-known types, reserving retired tags, and transactionally migrating persisted payloads. I found several behavioral regressions that need to be addressed before merge. The branch is also 23 commits behind main; GitHub currently reports unresolved conflicts in provider.rs, sandbox.rs, and ssh_sessions.rs.

  • Default sandbox exec immediately times out. Before this PR, the CLI's documented --timeout 0 bypassed timeout handling. The PR always sends Some(Duration::ZERO), causing both noninteractive and interactive commands to enter tokio::time::timeout(Duration::ZERO, ...) and normally return exit 124. Details: crates/openshell-cli/src/run.rs:1580-1585, :1958-1963; crates/openshell-server/src/grpc/sandbox.rs:2053-2068, :2134-2149.

  • Default inference routes get a zero-second timeout. Before this PR, --timeout 0 selected the 60-second default. The PR persists an explicit zero duration, which the supervisor treats as a real zero timeout. Ordinary inference requests can therefore fail immediately. Partial updates also replace an absent timeout with zero and truncate fractional durations. Details: crates/openshell-cli/src/run.rs:5489-5492, :5557-5583; crates/openshell-supervisor-network/src/inference_routes.rs:313-317.

  • Credential expiry can no longer be cleared. Before this PR, credential_expires_at_ms[key] = 0 removed an existing expiry. The new timestamp map has no per-key tombstone: omitting a key is a no-op, while epoch is a real, already-expired timestamp. The proto comment claiming an absent map entry removes the expiry cannot be implemented by the current merge. Details: proto/openshell.proto:1471-1477; crates/openshell-server/src/grpc/provider.rs:707-717; sdk/go/openshell/v1/internal/converter/provider.go:84-87.

  • Previously valid persisted conditions can block gateway startup. The old last_transition_time was an unconstrained driver-provided string. The migration now requires every nonempty value to parse as RFC 3339 and aborts the entire startup transaction otherwise. A custom driver value accepted by the previous release can therefore make the upgraded gateway unavailable. Details: crates/openshell-server/src/persistence/legacy_time_wire.rs:113-125.

  • Provider-profile durations bypass WKT validation and lose precision. Raw profile imports copy only Duration.seconds, ignoring nanos and malformed sign/range combinations. For example, 0.5s becomes zero and an invalid {seconds:1,nanos:-1} becomes one second. The Go converter additionally casts negative signed values to uint64, turning validation errors into absent/default durations. Details: crates/openshell-providers/src/profiles.rs:1026-1039, :1095-1111; sdk/go/openshell/v1/internal/converter/profile.go:191-198, :233-242.

  • Invalid log filters become unfiltered queries. Before this PR, the integer since_ms could not be structurally malformed. GetSandboxLogs now silently converts an invalid timestamp to zero and returns unfiltered history instead of INVALID_ARGUMENT; WatchSandbox correctly rejects the same input. Details: crates/openshell-server/src/grpc/policy.rs:4109-4119; compare crates/openshell-server/src/grpc/sandbox.rs:973-979.

  • The upgrade guide omits external protobuf peers. Compute drivers, credential drivers, middleware services, and sandbox supervisors also need coordinated upgrades. An old credential driver's tag-3 expiry is ignored by the new gateway, which then treats the credential as non-expiring. Details: docs/reference/protobuf-time-types.mdx:28-30; proto/credential_driver.proto:115-123; crates/openshell-server/src/credentials.rs:692-704.

  • Negative TypeScript exec timeouts become unlimited. Before this PR, encoding -1 into the old uint32 field failed. The new helper maps every nonpositive or nonfinite value to absence, allowing an invalid negative timeout to run indefinitely. Details: sdk/typescript/src/client.ts:35-37, :708, :789.

  • Agent guidance is stale. The TUI development skill still documents SandboxLogLine.timestamp_ms and GetSandboxLogsRequest.since_ms, so agents following it will generate obsolete code. Details: .agents/skills/tui-development/SKILL.md:493-494.

Branch CI is green, but all E2E lanes were skipped and the checks predate integration with current main. Focused Rust compilation and time/migration tests passed; Go, TypeScript, and Python SDK tests also passed. The missing cases are the sentinel, invalid-duration, expiry-removal, and legacy-string scenarios above.

@drew drew added the test:e2e Requires end-to-end coverage label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Label test:e2e applied for 62e0845. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(proto)!: use well-known timestamp and duration types

2 participants